-
-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft configuration setup #41
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking like it's going to be really useful. I have a few comments on the specific options but mostly I think let's split these up into separate PRs so that they can be reviewed more easily 🙂
.github/workflows/go.yml
Outdated
@@ -0,0 +1,28 @@ | |||
name: Go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stay with TravisCI for now 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.. sorry, it was rather unintentional 😕
config.go
Outdated
return c.maxInliningSize | ||
} | ||
|
||
func (c *Config) SetMaxInliningSize(maxInliningSize InlineSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (c *Config) SetMaxInliningSize(maxInliningSize InlineSize) { | |
func MaxItemsToInline(maxItems int) Configurator { |
I think the number of elements to inline makes sense to just be an int
rather than a separate type.
Also suggest we change this signature (and the other config options) so that callers can construct this like:
m := memviz.New(
MaxItemsToInline(10),
AnotherOption(false))
m.Map(&foo)
(i.e. using this pattern: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, awesome! I myself found the setter methods to be unfit for the init.
return m.mapPtrIface(iVal, inlineable, depth) | ||
// Collections | ||
case reflect.Struct: | ||
if m.config.maxDepth > 0 && depth > m.config.maxDepth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably lift this repeated depth check to just run once at the start of this function. If we're taking depth to mean "number of graphviz nodes" then I think we can use:
if depth >= m.config.maxDepth {
return 0, ""
}
i.e. if this value is going to cause another node to be created and we've already reached the limit then stop. This is perhaps a bit tricky because of the behaviour of inlineable
though.
I'd have to remind myself what exactly this does but I think it's more of a hint than an instruction so accurately calculating depth is tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we have interpreted �������depth
differently. 😆 When you say "number of graphviz nodes", do you mean a) in total? or b) the number of graphviz nodes down the embedded tree of the root object?
i.e. if this value is going to cause another node to be created and we've already reached the limit then stop. This is perhaps a bit tricky because of the behaviour of inlineable though.
I think this actually caught my attention during the first testing, and I worked around it. If it's inlineable it will still be rendered, maxdepth
"only" has an influence on next level objects. Please go in and test this, I will too, I am unclear on the details 😉
utils.go
Outdated
// writeDotStringToPng takes a content of a dot file in a string and makes a graph using Graphviz | ||
// to ouput an image | ||
// | ||
// sourced from https://github.com/Arafatk/DataViz/blob/master/utils/utils.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd rather just include a snippet in the README showing how to use this util function for those that want it rather than duplicating it here e.g.
b := &bytes.Buffer{}
memviz.Map(foo, b)
utils.WriteDotStringToPng(b.String(), "foo.png")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I only used for "visual testing", to be able to confirm the edits I made in the code. I have moved it to the README
links = append(links, fmt.Sprintf(" %d:f%d -> %d:name;\n", id, index, fieldID)) | ||
} else { | ||
// negative value means max depth was reached so don't link over to next node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm perhaps mapValue
should return something more clear like a bool saying that this was inlined/skipped.
I also think we should render something here to show that the field existed but was skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm perhaps
mapValue
should return something more clear like a bool saying that this was inlined/skipped.
I agree, this was a quick edit to test out the concept.
I also think we should render something here to show that the field existed but was skipped.
You mean "render" as in within the dot notation? Would that still count as "skipping"?
Hi @bradleyjkemp, this would be my initial take on the config... check it out and let me know